Skip to content

Add MemoryExtensions.Min/Max#128306

Open
manandre wants to merge 22 commits into
dotnet:mainfrom
manandre:span-min-max
Open

Add MemoryExtensions.Min/Max#128306
manandre wants to merge 22 commits into
dotnet:mainfrom
manandre:span-min-max

Conversation

@manandre
Copy link
Copy Markdown
Contributor

This pull request adds new Min and Max extension methods for ReadOnlySpan<T> to the System.Memory library, allowing users to efficiently find the minimum and maximum values in a span, optionally using a custom comparer. Comprehensive unit tests are included to verify correct behavior for various data types and edge cases.

New Min and Max methods for ReadOnlySpan<T>:

  • Added Min<T> and Max<T> extension methods to MemoryExtensions for ReadOnlySpan<T>, supporting both default and custom comparers, and handling nullable and reference types gracefully. These methods throw an exception on empty spans of non-nullable value types and return null for empty spans of reference or nullable types. [1] [2]

Unit tests:

  • Introduced comprehensive tests in ReadOnlySpan/MinMax.cs to cover empty spans, all-null values, custom comparers, and typical scenarios for both value and reference types.
  • Registered the new test file in the project file System.Memory.Tests.csproj to ensure the tests are run as part of the test suite.

Fixes #127083

Copilot AI review requested due to automatic review settings May 17, 2026 21:55
@dotnet-policy-service dotnet-policy-service Bot added the community-contribution Indicates that the PR has been added by a community member label May 17, 2026
@dotnet-policy-service
Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-memory
See info in area-owners.md if you want to be subscribed.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds Min and Max extension methods on ReadOnlySpan<T> (with optional IComparer<T>), mirroring LINQ's Enumerable.Min/Max semantics for spans. Includes ref assembly updates and tests.

Changes:

  • New Min/Max ReadOnlySpan extension methods with empty-span and null-element handling.
  • Reference assembly exposes the four new API overloads.
  • New test file covering empty, all-null, mixed-null, default comparer, and custom comparer scenarios.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.

File Description
src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.cs Implements Min/Max with a generic comparer fast path and null-skipping inner core.
src/libraries/System.Memory/ref/System.Memory.cs Adds public surface for the four new overloads.
src/libraries/System.Memory/tests/System.Memory.Tests.csproj Includes the new test file.
src/libraries/System.Memory/tests/ReadOnlySpan/MinMax.cs Tests for empty/null/normal/custom-comparer behaviors.

Comment thread src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.cs Outdated
Comment thread src/libraries/System.Memory/tests/ReadOnlySpan/MinMax.cs
Comment thread src/libraries/System.Memory/ref/System.Memory.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.cs Outdated
@MihaZupan
Copy link
Copy Markdown
Member

Enumerable's Min/Max already have vectorized logic implementing this. We should move that logic into these new methods, then switch LINQ to call them instead

Copilot AI review requested due to automatic review settings May 18, 2026 22:00
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.

Comment thread src/libraries/System.Private.CoreLib/src/Resources/Strings.resx Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.cs Outdated
Comment thread src/libraries/System.Memory/ref/System.Memory.cs Outdated
Comment thread src/libraries/System.Memory/tests/ReadOnlySpan/MinMax.cs Outdated
Comment thread src/libraries/System.Memory/tests/ReadOnlySpan/MinMax.cs Outdated
Copilot AI review requested due to automatic review settings May 18, 2026 22:55
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.

Comment thread src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.cs Outdated
Comment thread src/libraries/System.Memory/tests/ReadOnlySpan/MinMax.cs
Comment thread src/libraries/System.Memory/tests/ReadOnlySpan/MinMax.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.cs Outdated
/// </remarks>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static T? Min<T>(this ReadOnlySpan<T> span) =>
Min(span, Comparer<T>.Default);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much like with LINQ, this should be passing comparer: null and then we should be treating a null comparer as selecting the default, not throwing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tannergooding Should we then update the approved API to explicitly accept a null comparer?

    public T? Min<T>(this ReadOnlySpan<T> span, IComparer<T>? comparer);
    public T? Max<T>(this ReadOnlySpan<T> span, IComparer<T>? comparer);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nullability annotations are often overlooked in the API review. IComparer<T> parameters is a case that we almost universally allow to be nullable (much like IFormattable, IEqualityComparer, and a couple others) and where we often explicitly want it to be so we can do specific optimizations.

I've left an explicit comment indicating the oversight, so please adjust the PR accordingly.

/// <para>If <typeparamref name="T" /> is a reference type and the span sequence is empty, this method returns <see langword="null" />.</para>
/// <para>Null values are ignored when determining the minimum value. If the span contains at least one non-null value, the minimum of those values is returned. If the span does not contain any non-null values, <see langword="null" /> is returned.</para>
/// </remarks>
public static T? Min<T>(this ReadOnlySpan<T> span, IComparer<T> comparer)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unclear why this deviates from the enumerable logic in implementation.

I'd generally expect the two entry points to be nearly identical. Covering the acceleration and other checks like: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Linq/src/System/Linq/Min.cs#L292-L373

The biggest difference should be that if (!enumerator.MoveNext()) becomes if (i >= span.Length). The JIT should already know that span.Length can't be negative and should know the same of i since we initialize it to 0 and only ever do ++

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have aligned the implementation on the enumerable logic.

Copilot AI review requested due to automatic review settings May 19, 2026 21:49
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Comment thread src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.cs Outdated
Copilot AI review requested due to automatic review settings May 19, 2026 22:21
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Comment thread src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.cs Outdated
Comment thread src/libraries/System.Memory/tests/ReadOnlySpan/MinMax.cs
Copilot AI review requested due to automatic review settings May 19, 2026 22:41
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Comment thread src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.cs Outdated
Comment thread src/libraries/System.Memory/tests/ReadOnlySpan/MinMax.cs
Copy link
Copy Markdown
Member

@MihaZupan MihaZupan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the duplicate logic in Linq and call these new helpers instead.

Comment thread src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.cs Outdated
ref Unsafe.As<TFrom, TTo>(ref MemoryMarshal.GetReference(span)),
span.Length);

private interface IMinMaxDirection
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we reuse the other interface we're adding instead of having two sets?

Copy link
Copy Markdown
Contributor Author

@manandre manandre May 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not manage to do it.
IMinMaxCalc<T> where T : struct, IBinaryInteger<T> is a generic interface for integer types and so is not compatible with the IMinMaxDirection one.

Comment thread src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.cs Outdated
Comment thread src/libraries/System.Linq/src/System/Linq/MaxMin.cs Outdated
Comment thread src/libraries/System.Linq/src/System/Linq/MaxMin.cs Outdated
Copilot AI review requested due to automatic review settings May 20, 2026 21:15
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

Comment thread src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.MinMax.cs Outdated
Comment thread src/libraries/System.Memory/tests/ReadOnlySpan/MinMax.cs
{
public static partial class Enumerable
{
private interface IMinMaxCalc<T> where T : struct, IBinaryInteger<T>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there still a benefit to having this logic?
It seems like we could remove it any change the caller to use the span.Min directly instead.
E.g. replace this:

// TODO https://github.com/dotnet/csharplang/discussions/6308: Update this to use generic constraint bridging if/when available.
if (typeof(TSource) == typeof(byte) && comparer == Comparer<TSource>.Default) return (TSource)(object)MinMaxInteger<byte, MinCalc<byte>>((IEnumerable<byte>)source);
if (typeof(TSource) == typeof(sbyte) && comparer == Comparer<TSource>.Default) return (TSource)(object)MinMaxInteger<sbyte, MinCalc<sbyte>>((IEnumerable<sbyte>)source);
if (typeof(TSource) == typeof(ushort) && comparer == Comparer<TSource>.Default) return (TSource)(object)MinMaxInteger<ushort, MinCalc<ushort>>((IEnumerable<ushort>)source);
if (typeof(TSource) == typeof(short) && comparer == Comparer<TSource>.Default) return (TSource)(object)MinMaxInteger<short, MinCalc<short>>((IEnumerable<short>)source);
if (typeof(TSource) == typeof(char) && comparer == Comparer<TSource>.Default) return (TSource)(object)MinMaxInteger<char, MinCalc<char>>((IEnumerable<char>)source);
if (typeof(TSource) == typeof(uint) && comparer == Comparer<TSource>.Default) return (TSource)(object)MinMaxInteger<uint, MinCalc<uint>>((IEnumerable<uint>)source);
if (typeof(TSource) == typeof(int) && comparer == Comparer<TSource>.Default) return (TSource)(object)MinMaxInteger<int, MinCalc<int>>((IEnumerable<int>)source);
if (typeof(TSource) == typeof(ulong) && comparer == Comparer<TSource>.Default) return (TSource)(object)MinMaxInteger<ulong, MinCalc<ulong>>((IEnumerable<ulong>)source);
if (typeof(TSource) == typeof(long) && comparer == Comparer<TSource>.Default) return (TSource)(object)MinMaxInteger<long, MinCalc<long>>((IEnumerable<long>)source);
if (typeof(TSource) == typeof(nuint) && comparer == Comparer<TSource>.Default) return (TSource)(object)MinMaxInteger<nuint, MinCalc<nuint>>((IEnumerable<nuint>)source);
if (typeof(TSource) == typeof(nint) && comparer == Comparer<TSource>.Default) return (TSource)(object)MinMaxInteger<nint, MinCalc<nint>>((IEnumerable<nint>)source);
if (typeof(TSource) == typeof(Int128) && comparer == Comparer<TSource>.Default) return (TSource)(object)MinMaxInteger<Int128, MinCalc<Int128>>((IEnumerable<Int128>)source);
if (typeof(TSource) == typeof(UInt128) && comparer == Comparer<TSource>.Default) return (TSource)(object)MinMaxInteger<UInt128, MinCalc<UInt128>>((IEnumerable<UInt128>)source);

with

if (source.TryGetSpan(out ReadOnlySpan<T> span))
{
    return span.Min(comparer);
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice optimization indeed!

Copy link
Copy Markdown
Member

@MihaZupan MihaZupan May 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to delete this file and IMinMaxCalc from Linq, no?

Comment on lines +27 to +28
public static T? Min<T>(this ReadOnlySpan<T> span) =>
Min(span, comparer: null);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that this overload (no comparer) is the expected fast path, it may be better to swap the calls so that it looks more like this instead:

public static T? Min<T>(this ReadOnlySpan<T> span)
{
    if (typeof(T) == typeof(byte)) return MinMaxInteger<T, byte, MinCalc<byte>>(span);
    if (typeof(T) == typeof(sbyte)) return MinMaxInteger<T, sbyte, MinCalc<sbyte>>(span);
    // ...

    return MinMax<T, MinDirection>(span, Comparer<T>.Default);
}

public static T? Min<T>(this ReadOnlySpan<T> span, IComparer<T>? comparer)
{
    if (comparer is null || comparer == Comparer<T>.Default)
    {
        return Max(span);
    }

    return MinMax<T, MinDirection>(span, comparer);
}

So the caller is pretty much guaranteed to call into just the core helper.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, it should reduce comparer checks.
I have removed the AgressiveInlining option as it does not seem relevant anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-System.Memory community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[API Proposal]: MemoryExtensions.Min/Max

5 participants